ION Reads and Writes #370

decentralgabe · 2023-04-01T00:59:29Z

Overview

Addresses part of ION Reads & Writes #340 to add create and get ops for ION DIDs
Made DID storage more generic; switched the stored DID to an interface type and added tests for this
Fixes Generic "option" field that has unique meaning per DID method #335 by adding options per DID create method
Removed unnecessary method param for each handler
Added a number of TODOs

* origin/main: [OSE-433] Add the OpenID4VC authorization endpoint (#338) Moarrr docs (#363) # Conflicts: # go.mod # go.sum

codecov-commenter · 2023-04-01T01:09:56Z

Codecov Report

Merging #370 (6cf039a) into main (e206a33) will increase coverage by 0.22%.
The diff coverage is 30.55%.

@@            Coverage Diff             @@
##             main     #370      +/-   ##
==========================================
+ Coverage   20.23%   20.46%   +0.22%     
==========================================
  Files          42       48       +6     
  Lines        4675     5454     +779     
==========================================
+ Hits          946     1116     +170     
- Misses       3597     4172     +575     
- Partials      132      166      +34

Impacted Files	Coverage Δ
config/config.go	`16.14% <0.00%> (ø)`
integration/common.go	`0.00% <0.00%> (ø)`
internal/util/util.go	`0.00% <0.00%> (ø)`
pkg/authorizationserver/oauth2.go	`81.42% <ø> (ø)`
pkg/server/router/did.go	`4.84% <0.00%> (-1.71%)`	⬇️
pkg/server/router/keystore.go	`10.25% <ø> (ø)`
pkg/service/did/handler.go	`0.00% <ø> (ø)`
pkg/service/did/key.go	`0.00% <0.00%> (ø)`
pkg/service/did/resolution/resolver.go	`0.00% <0.00%> (ø)`
pkg/service/did/service.go	`0.00% <0.00%> (ø)`
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

nitro-neal

Great job! Just a few little things and some clarifying questions

nitro-neal · 2023-04-03T15:47:05Z

config/config.go

-			Methods:           []string{"key", "web"},
-			ResolutionMethods: []string{"key", "peer", "web", "pkh"},
+			BaseServiceConfig:      &BaseServiceConfig{Name: "did"},
+			Methods:                []string{"key", "web"},


do we want to add ion into the default method?

I don't think so because it requires having a valid write node configured

nitro-neal · 2023-04-03T15:48:29Z

config/compose.toml

 universal_resolver_url = "http://uni-resolver-web:8080"
 universal_resolver_methods = ["ion"]
+ion_resolver_url = "https://tbdwebsiteonline.com"


url doesn't resolve to anything right now, was this a placeholder or will this be the real url?

this is in fact our ion node - you'll notice it works if you make a query like https://tbdwebsiteonline.com/identifiers/did:ion:EiBnKzBTMxW6wM5kIlVBHidpbvKHC9QrNqfYL-Rhpbt63Q

nitro-neal · 2023-04-03T15:48:58Z

integration/testdata/did-web-input.json

@@ -1,4 +1,6 @@
 {
  "keyType":"Ed25519",
-  "didWebId":"did:web:tbd.website"
+  "options": {


nitro-neal · 2023-04-03T15:59:28Z

pkg/server/router/did.go

@@ -128,6 +133,56 @@ func (dr DIDRouter) CreateDIDByMethod(ctx context.Context, w http.ResponseWriter
 	return framework.Respond(ctx, w, resp, http.StatusCreated)
 }

+// toCreateDIDRequest converts CreateDIDByMethodRequest to did.CreateDIDRequest, parsing options according to method
+func toCreateDIDRequest(m didsdk.Method, request CreateDIDByMethodRequest) (*did.CreateDIDRequest, error) {


pretty clever! Was this overkill though?

What was the reason to not have just string fields in the option{} req object?

*didKey -> option better not exist or return err

*didWeb -> option better be url or return err

*didIon -> option better not exist or return err

help me understand...the option object can accept anything - for ion need to accept an array of public keys or service endpoints

the object is a generic any but to extract the value you need to know the concrete type

if it can be simplified let's do it I couldn't figure out a simpler way

type CreateDIDByMethodRequest struct {

KeyType crypto.KeyType `json:"keyType" validate:"required"` // Required when creating a DID with the `web` DIDWebID string `json:"didWebId"` // Required when creating a DID with the `ion` DIDIonPublicKeys []string `json:"didIonPublicKeys"` DIDIonServiceEndpoints []string `json:"didIonServiceEndpoints"`

}

^ could be like this instead right?

I think I like your way more, but just wonder if it was overkill and could introduce some bugs or efficiency issue

oh yeah, this can work. the tradeoff is then having an "any" request vs a more generic method like I have and specific impls per type...with then the tradeoff of having some ugly mapping code.

I think the real solution is to get away from the generic -> specific funnel code we have now and instead register first-party handlers...I'm not entirely sure how this would work it would be a significant refactor but could be a more natural approach than forcing all DID methods to follow a similar structure.

I think it's related to Andres's points here #362 so I'll add a note with your comment and we can address in there

nitro-neal · 2023-04-03T16:00:53Z

pkg/server/router/keystore.go

 	Type crypto.KeyType `json:"type,omitempty" validate:"required"`

 	// See https://www.w3.org/TR/did-core/#did-controller
 	Controller string `json:"controller,omitempty" validate:"required"`

 	// Base58 encoding of the bytes that result from marshalling the private key using golang's implementation.
-	PrivateKeyBase58 string `json:"base58PrivateKey,omitempty" validate:"required"`
+	PrivateKeyBase58 string `json:"base58PrivateKey,omitempty"`


remove validate:"required" was this intendend ?

I can add this back until #371

nitro-neal · 2023-04-03T16:07:37Z

pkg/service/did/ion.go

+		ID:          ionDID.ID(),
+		DID:         didDoc,
+		SoftDeleted: false,
+		LongFormDID: ionDID.LongForm(),


can these get really really really big? Maybe we need to revist the max size redis can store

yeah I think this could end up being a few MB with updates. for now it should be fine

nitro-neal · 2023-04-03T16:09:28Z

pkg/service/did/ion.go

+	logrus.WithError(err).Warnf("error getting DID from storage: %s", id)
+
+	// if not, resolve it from the network
+	resolved, err := h.resolver.Resolve(ctx, id, nil)


nitro-neal · 2023-04-03T16:10:39Z

pkg/service/did/ion.go

+func (h *ionHandler) GetDID(ctx context.Context, request GetDIDRequest) (*GetDIDResponse, error) {
+	id := request.ID
+
+	// TODO(gabe) as we are fully custodying ION DIDs this is fine; as we move to a more decentralized model we will


maybe like we could at least have a log line / throw an error if the did in storage and the did that gets resolved are different?

for now this is impossible since we only allow create and do not return the update key to the user. when we add updates and/or self-custody operations we will need to do this

nitro-neal · 2023-04-03T16:11:02Z

pkg/service/did/ion.go

+	return &GetDIDResponse{DID: resolved.Document}, nil
+}
+
+// GetDIDs returns all DIDs we have in storage for ION, it is not feasible to get all DIDs from the network


there is no Ion batch call functionality ?

looking at the api - no - this is an improvement we can make

decentralized-identity/ion#319

nitro-neal · 2023-04-03T16:15:21Z

pkg/service/did/storage.go

 	for _, didBytes := range gotDIDs {
-		var nextDID StoredDID
+		nextDID := reflect.New(reflect.TypeOf(outType).Elem()).Interface()


this could be computationally really slow? (maybe not)

yeah, depending on how large the set of DIDs is this could be bad. this will mostly be fine with #349

nitro-neal · 2023-04-03T18:34:24Z

🏆

gabe added 5 commits March 29, 2023 17:24

temp

9a0bdea

ion did progress

48aa736

generic DID storage

a37cbce

Merge remote-tracking branch 'origin/main' into 340-ion-reads-writes

ff199eb

* origin/main: [OSE-433] Add the OpenID4VC authorization endpoint (#338) Moarrr docs (#363) # Conflicts: # go.mod # go.sum

merge'

3c4a426

gabe added 6 commits March 31, 2023 18:48

ion logic

2759457

temp:

d671331

test ion did handler in unit test

dc4e6cd

integration test

982128a

lint

fee399f

whoops

0f0d429

decentralgabe marked this pull request as ready for review April 1, 2023 05:44

decentralgabe requested review from nitro-neal and andresuribe87 as code owners April 1, 2023 05:44

decentralgabe added this to the BTC Miami milestone Apr 1, 2023

decentralgabe added the did label Apr 1, 2023

decentralgabe self-assigned this Apr 1, 2023

gabe added 10 commits March 31, 2023 23:05

add ion to config

ba47a2c

better tests

422178d

fix tests

01b4410

return priv key bytes

18344fc

update comment

86607fc

refer to alice's key properly

5ebd390

fix int test

7b9cb47

options

6c9804d

more test fix

13efea7

lint

4a0d02f

nitro-neal approved these changes Apr 3, 2023

View reviewed changes

pr comment

368a076

gabe and others added 2 commits April 3, 2023 11:47

fix go sum

563992d

Merge branch 'main' into 340-ion-reads-writes

6cf039a

decentralgabe merged commit 35eb2e4 into main Apr 3, 2023

decentralgabe deleted the 340-ion-reads-writes branch April 3, 2023 18:59

decentralgabe mentioned this pull request Apr 3, 2023

ION Reads & Writes #340

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ION Reads and Writes #370

ION Reads and Writes #370

decentralgabe commented Apr 1, 2023

codecov-commenter commented Apr 1, 2023 •

edited

Loading

nitro-neal left a comment

nitro-neal Apr 3, 2023

decentralgabe Apr 3, 2023

nitro-neal Apr 3, 2023

decentralgabe Apr 3, 2023

nitro-neal Apr 3, 2023

nitro-neal Apr 3, 2023

decentralgabe Apr 3, 2023

nitro-neal Apr 3, 2023

decentralgabe Apr 3, 2023

decentralgabe Apr 3, 2023

nitro-neal Apr 3, 2023

decentralgabe Apr 3, 2023

nitro-neal Apr 3, 2023

decentralgabe Apr 3, 2023

nitro-neal Apr 3, 2023

nitro-neal Apr 3, 2023

decentralgabe Apr 3, 2023

nitro-neal Apr 3, 2023

decentralgabe Apr 3, 2023

decentralgabe Apr 3, 2023

nitro-neal Apr 3, 2023

decentralgabe Apr 3, 2023

nitro-neal commented Apr 3, 2023

ION Reads and Writes #370

ION Reads and Writes #370

Conversation

decentralgabe commented Apr 1, 2023

Overview

codecov-commenter commented Apr 1, 2023 • edited Loading

Codecov Report

nitro-neal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitro-neal commented Apr 3, 2023

codecov-commenter commented Apr 1, 2023 •

edited

Loading